Skip to content

fix: confirm extraction history item deletion#2261

Open
itsbryanman wants to merge 2 commits into
nexu-io:mainfrom
itsbryanman:fix/extraction-history-delete-confirmation-2249
Open

fix: confirm extraction history item deletion#2261
itsbryanman wants to merge 2 commits into
nexu-io:mainfrom
itsbryanman:fix/extraction-history-delete-confirmation-2249

Conversation

@itsbryanman
Copy link
Copy Markdown

@itsbryanman itsbryanman commented May 19, 2026

Fixes #2249

Why

Deleting an individual extraction history item in Settings → Memory → Extraction history happened immediately after clicking the row delete control.

That made it too easy for a user to accidentally remove their own extraction history data with no confirmation, warning, or undo path. This PR adds a confirmation step for the individual row delete flow so the delete action only runs after explicit confirmation.

What users will see

In Settings → Memory → Extraction history, clicking the delete control on a single history item now opens a confirmation dialog.

The item is not deleted unless the user confirms the action. Canceling or dismissing the dialog leaves the history item unchanged.

Surface area

  • UI — new page / dialog / panel / menu item / setting / empty state in apps/web or apps/desktop (including Electron menu bar)
  • Keyboard shortcut — new or changed
  • CLI / env var — new od subcommand or flag, new tools-dev / tools-pack / tools-pr flag, or new OD_* env var
  • API / contract — new /api/* endpoint, new SSE event, or changed shape in packages/contracts
  • Extension point — new entry under skills/, design-systems/, design-templates/, or craft/, or change to the skills protocol
  • i18n keys — added new translation keys
  • New top-level dependency — adding any new entry to the root package.json
  • Default behavior change — changes what existing users experience without opting in
  • None — internal refactor, docs, tests, or translation update only

Screenshots

Not attached.

The visible change is a confirmation dialog shown after clicking the delete control for an individual extraction history item.

Bug fix verification

  • Test path that reproduces the bug:

    • apps/web/tests/components/MemorySection.test.tsx

  • Added/updated tests covering:

    • Clicking the individual row delete control does not immediately call the delete handler/API.
    • The confirmation dialog appears after clicking delete.
    • Canceling the dialog does not delete the item.
    • Confirming the dialog deletes only the selected extraction history item.
    • Failed deletion keeps/restores the item instead of silently removing it.

  • Did the test go red on main and green on this branch?

    • Not separately verified on main.
    • The updated targeted test suite passes on this branch.

Validation

Passed locally:

  • ./node_modules/.bin/vitest run -c vitest.config.ts tests/components/MemorySection.test.tsx

    • Result: 20 passed

  • ./node_modules/.bin/tsc -b --noEmit

    • Result: passed

Lint:

  • apps/web doesnt define a lint script

@lefarcen lefarcen requested a review from nettee May 19, 2026 11:38
@lefarcen lefarcen added size/M PR changes 100-300 lines risk/medium Medium risk: regular code changes type/bugfix Bug fix labels May 19, 2026
@lefarcen lefarcen mentioned this pull request May 19, 2026
Open
nettee
nettee requested changes May 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@nettee nettee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one blocking issue in the updated test coverage and one follow-up i18n concern.

🔁 Powered by Looper · runner=reviewer · agent=opencode · model=openai/gpt-5.4 · An autonomous AI dev team for your GitHub repos.

Comment thread apps/web/tests/components/MemorySection.test.tsx Outdated
Comment thread apps/web/src/components/MemorySection.tsx Outdated
@itsbryanman itsbryanman force-pushed the fix/extraction-history-delete-confirmation-2249 branch from 1a61c0b to 587f5dc Compare May 19, 2026 12:20
@lefarcen lefarcen requested a review from nettee May 19, 2026 12:27
nettee
nettee requested changes May 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@nettee nettee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one blocking regression in the new confirmation flow and one non-blocking localization follow-up.

🔁 Powered by Looper · runner=reviewer · agent=opencode · model=openai/gpt-5.4 · An autonomous AI dev team for your GitHub repos.

Comment thread apps/web/src/i18n/locales/en.ts Outdated
Comment thread apps/web/src/components/MemorySection.tsx Outdated
@itsbryanman itsbryanman force-pushed the fix/extraction-history-delete-confirmation-2249 branch from 587f5dc to 6b22b15 Compare May 19, 2026 12:51
@lefarcen lefarcen requested a review from nettee May 19, 2026 12:59
nettee
nettee requested changes May 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@nettee nettee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one blocking regression in the new confirmation flow.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

Comment thread apps/web/src/components/MemorySection.tsx
@lefarcen lefarcen requested a review from qiongyu1999 May 20, 2026 03:09
nettee
nettee requested changes May 20, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@nettee nettee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one blocking regression in the new confirmation flow.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

Comment thread apps/web/src/components/MemorySection.tsx
if (!pendingExtractionDeleteId) return;
setIsDeletingExtraction(true);
try {
await onDeleteExtraction(pendingExtractionDeleteId);
Copy link
Copy Markdown
Contributor

@nettee nettee May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At apps/web/src/components/MemorySection.tsx:1361, the new confirm path still lets a rejected DELETE leave the row optimistically hidden. onDeleteExtraction() removes the row before awaiting deleteExtraction(id), and this await onDeleteExtraction(...) call only handles the ok === false case—if fetch() rejects (for example because the daemon is offline), the rejection escapes, the dialog stays open, and the item is only recoverable after a manual reload. The added tests in apps/web/tests/components/MemorySection.test.tsx cover a 500 response but never a rejected promise, so the PR body claim that failed deletion keeps or restores the item is still unmet for this failure mode. Please catch rejected deletes here (or inside onDeleteExtraction), restore or reload the extraction list on failure, and add a test where the mocked DELETE request rejects.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lefarcen lefarcen lefarcen left review comments

@nettee nettee nettee requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

risk/medium Medium risk: regular code changes size/M PR changes 100-300 lines type/bugfix Bug fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deleting an extraction history item does not require confirmation

3 participants

@itsbryanman @nettee @lefarcen